I don't like having to copy the local *.conf and board/ files into the new sample, but a general pattern must work across modules. Once Zephyr PR #47638 is fully approved and merged, then these files could be referenced rather than copied.

This PR implementation has been cleaned up and existing functional builds verified and documented at every commit. (NOTE: Make-ing mcuboot/sample/zephyr did not succeed at the base of this PR, so it was not part of the verification process.)

The push above takes advantage of the just-merged of the fix for Zephyr's issue #41830, enabling use of ZEPHYR__MODULE_DIR in CONF_FILE, OVERLAY_CONFIG, and DTC_OVERLAY_FILE before find_package().

It also takes advantage of the APPLICATION_CONFIG_DIR.

Together these reduced that solution to 17 files (touched and created).

The following changes address comments above:

• The Zephyr build issue has been captured in APPLICATION_CONFIG_DIR, CONF_FILE do not always pick up local boards/*.conf zephyrproject-rtos/zephyr#51621.
• mcuboot_svc has been renamed mcuboot_app
• The mcuboot/boot/zephyr/CMakeLists.txt logic has been extracted out into files in mcuboot/zephyr/cmake/.
• The new sample has been relocated to mcuboot/zephyr/samples/mcuboot_external_config/

I think that addresses all feedback so far.

@nvlsianpu Since you missed today's TSC meeting where we reviewed the minimum implementation in detail, do you have any feedback?

@nordicjm What is the command line/test case you were building that shows the warning?

@nordicjm What is the command line/test case you were building that shows the warning?

cmake -GNinja -DBOARD=nrf52dk_nrf52832 ..

cmake -GNinja -DBOARD=nrf52dk_nrf52832 ..

Which was built from where?

[DNM] This need be considered by zephyr-rtos/zephyr maintainers

Why?!? Notice:

• MCUboot is a downstream project from Zephyr, dependency-wise. (Even Anas has described it that way). It is also downstream of multiple other OS'.
• Zephyr has already elected to provide a fork of the MCUboot module. In this Zephyr Project PR comment Anas describes exactly why Zephyr has a fork - so Zephyr can make changes as they deem necessary.
• All the changes here are in alignment with the current Zephyr build system, patterns illustrated or implied in Zephyr's example-application repository.
• All the changes here have already been verified to work with all the Zephyr-provided test suites that I am aware are relevant.
• Zephyr's SOF module fork has already started to put the Zephyr directory tree within $module/zephyr/.
• Zephyr's current MAINTAINERS file lists the maintainers as d3zd3z, de-nordic, nordicjm. These have already participated in the review.

In other words, they have effectively already considered and approved these types of changes, and it is the responsibility of the Zephyr mcuboot fork maintainer to adjust their fork as necessary when they update the fork with changes from the upstream project.

(Edit 1 start)
Please remove the baseless [DNM].

If the goal of DNM is to get Zephyr Project feedback toward being a recommended general OOT module pattern, then please put a short time limit on DNM and identify which Zephyr working group will be approached for the feedback. Rolling out initial Zephyr-based testing of mcuboot logic is unblocked by this PR, so we shouldn't wait long.

(Edit 1 end)

@gregshue I see your points on that mcuboot is a downstream project for zephyr. Having a fork for mcuboot in zephyr-rtos GH doesn't determine how the maintenance is preformed. I want to have 1:1 relation, so fare this approach have been working fine. On every synchronization of zephyr-rtos/mcuboot with the origin the code diff is expected to be empty.

I design this PR with DNM as this change have critical severity for the zephyr-project and every projects which leverage the mcuboot zephyr port.

As you can see we have already review from build system expert.

Nothing in this change breaks the zephyr builds. As Ana's has said, Zephyr needs to make decisions independent of downstream concerns. In fact, this change came about because the current implementation does not fully support the range of user needs supported by Zephyr.

Specifically, What changes here are critically severe?

Nothing in this change breaks the zephyr builds.

In which case any PR should just blindly be approved, or what is your point ?

This PR is not simply trying to improve current code, but moving code around irrespective of the current design.
Although part of existing code can surely be improved, the move proposed in this PR just makes things even more messy.

As Ana's has said, Zephyr needs to make decisions independent of downstream concerns.

and that is a carte-blanche to request changes directly in the mcuboot repository, or what is your point ?
Try to compare the change @nashif is requesting, which is a single config line in a prj.conf to the major changes requested in this PR.

In fact, this change came about because the current implementation does not fully support the range of user needs supported by Zephyr.

Exactly which use-case is not properly handled by the description provided here:
#1489 (review)

and why can such use-case not be done in a less-intrusive way ?

Maybe I have missed something fundamental regarding this PR, ...

You have.

I urge @gregshue to instead create much smaller PRs with a clear objective of what is the exact goal.

Good suggestion. Look at PR #1524

MCUboot is the manifest repository of my workspace. Building this this PR from the root of my workspace with either current mcuboot main branch or after manually applying the changes of #1522 generates the same build failures.

'''
west build -p always -b nrf52840dk_nrf52840 mcuboot/zephyr/samples/modules/mcuboot/mcuboot_external_config/
'''

gives two sets of warnings (even though building mcuboot/boot/zephyr builds cleanly):

• (1x) warning: MBEDTLS_CFG_FILE (defined at modules/mbedtls/Kconfig:50, modules/mbedtls/Kconfig:50) was assigned the value 'mcuboot-mbedtls-cfg.h' but to the value ''. Check these unsatisfied dependencies: ((MBEDTLS_BUILTIN && MBEDTLS) || (MBEDTLS_BUILTIN && MBEDTLS && 0)) (=n).
• (12x) warning: attempt to assign the value 'n' to the undefined symbol ...

I cannot even reach the point of verifying my local board settings cascade with those in APPLICATION_CONFIG_DIR.

MCUboot is a Zephyr module containing the MCUboot application.

The MCUboot zephyr module also contains the mcuboot bootutil library.

The current design builds a bootloader independent of the application.
With your design changes, then you start building and linking the application together with the bootloader in the process, and thus they start to be tightly coupled with all the cons of address spacing and separating the two parts in the linker script.

This is where you misunderstood what this does. This still builds and flashes the bootloader independently of the application. According to the zephyr.map files, this refactoring only does the following w.r.t. linking:

• app/libapp.a is reduced to have keys.c.obj. All the other object files go into zephyr/libzephyr.a.
• mcuboot/libmcuboot_util.a becomes mcuboot/subsys/mcuboot_util/libmcuboot_util.a.

This is an application specific CMakeLists.txt file suddenly being moved into Zephyr module CMake processed code.

AFAICT the only part of the original CMakeLists.txt that makes this an application CMakeLists.txt is 'find_project(Zephyr ...)'. The rest could be refactored into include()ed files, or into subsystems. From my many years of experience in this area, this will be much easier to maintain if the "application" logic is refactored as a subsystem.

just moving the code really doesn't improve anything.

... except for fixing the use cases that the current configuration doesn't support.

In which case any PR should just blindly be approved, or what is your point ?

The answer didn't identify what changes "have critical severity for the zephyr-project". It doesn't seem to be a build issue.

but moving code around irrespective of the current design.

The current design doesn't meet the breadth of user needs (e.g., mine, as shown by the samples).

the move proposed in this PR just makes things even more messy.

I actually find it far more orderly, consistent, scalable, and applicable to other downstream modules than the current design.

and why can such use-case not be done in a less-intrusive way ?

It may be able to be done in a less intrusive way, but this is a way that follows existing patterns in Zephyr and example-application, introduces more alignment with the Zephyr build/configuration system, and can be applied in general to downstream repositories.

Show me another way to meet this use case and then let's evaluate the approaches based on overall merit.

MCUboot is the manifest repository of my workspace

Then your project is completely wrong from the start.

I urge @gregshue to instead create much smaller PRs with a clear objective of what is the exact goal.

Good suggestion. Look at PR #1524

Thanks.

The linked PR (#1524) actual reveals that this PR (#1489) originates from a lack of understanding on how MCUboot is integrated into Zephyr and how the build system (CMake and Kconfig) it works.

Then your project is completely wrong from the start.

That statement directly contradicts Zephyr support for T2 topology and what is done in Zephyr Project's example-application repository. Try again.

The linked PR (#1524) actual reveals that this PR (#1489) originates from a lack of understanding on how MCUboot is integrated into Zephyr and how the build system (CMake and Kconfig) it works.

Actually, you have this backwards. The linked PR is a use case that the MCUboot integration must support (i.e., requirement) - though I think top-level CMakeLists.txt files should never be include()ed in other projects.

Actually, you have this backwards. The linked PR is a use case that the MCUboot integration must support

did you read the comments I wrote and tried it out ? (#1524 (review))
The use-case of configuring MCUboot externally is possible, I even created a branch for you.
https://github.com/tejlmand/mcuboot/tree/mcuboot/review_1524

If you want to wrap that inside a CMakeLists.txt file, then that is also possible and works as expected.
Just understand how the system is intended to work.

though I think top-level CMakeLists.txt files should never be include()ed in other projects.

So use ExternalProject_Add() or add_subdirectory().
There exists several ways, but this (#1489) is a PR under review and not about helping you to understand the system.
So now we back at the fact that your use-case is supported:

• You can use APPLICATION_CONFIG_DIR to place configuration files for MCUboot out-of-tree of the existing MCUboot folder.
  That oot-of-tree can even contain your own and custom board configuration
• You can create your own custom CMakeLists.txt file and include the Zephyr MCUboot port as include(), add_subdirectory(), or the more general ExternalProject_Add() which is used when including an external project.
• You can create your own custom CMakeLists.txt and fully control MCUboot any way you like, just like espressif does:
  https://github.com/zephyrproject-rtos/mcuboot/blob/main/docs/readme-espressif.md
  Customizing a MCUboot like that even works with Zephyr, as seen here:
  soc: esp32: Support booting from MCUboot zephyrproject-rtos/zephyr#37872

So there are plenty of ways that you can customize MCUboot.
Notice, the sample in https://github.com/zephyrproject-rtos/mcuboot/tree/main/boot/zephyr is a reference sample on how Zephyr integrates MCUboot, but you can do that in a myriad different ways.

Either by using ways already supported by the Zephyr CMake / Kconfig build and configuration system which even allows a large degree of freedom on how, copy the MCUboot sample to own repo and make adjustments or go completely independent of the Zephyr build system, like espressif case, and still build MCUboot so that it works with Zephyr.

But #1489 is a PR where you propose yet one more variant in customization.
It is not a guideline on how to carry out above possibilities.

I keep my 👎 on this PR as I don't see the need to support and maintain yet one more way of integrating MCUboot with Zephyr.
One more way that risks confusing users on how to use the system.
More risk of corner cases.
One more way that must be tested, documented, maintain.

MCUboot and MCUboot with Zephyr is already very flexible and support large degree of customization (although documentation on how to actually do this can probably be improved).

did you read the comments I wrote and tried it out ?

Apparently not well enough. Now that I have here's what I found:

• I successfully built from my mcuboot-manifested workspace with the following command:
  '''
  west build -p always -b nrf52840dk_nrf52840 mcuboot/boot/zephyr
  -DAPPLICATION_CONFIG_DIR=(pwd)/mcuboot/samples/zephyr/mcuboot_external_configuration
  '''

• I adding a local CMakeLists.txt (include(${CMAKE_CURRENT_LIST_DIR}/../../../boot/zephyr/CMakeLists.txt)` and running the following command:
  '''
  west build -p always -b nrf52840dk_nrf52840 mcuboot/samples/zephyr/mcuboot_external_configuration
  '''
  The build FAILED, reporting warnings on:

  1. Kconfigs being assigned non-'n' values but having value 'n'
  2. attempt to assign the value 'n' to the undefined symbol ...;

• I copied $mcuboot/boot/zephyr/Kconfig locally, fixed up the rsource "Kconfig.serial_recovery" to include the relative path, and attempted the command immediately above. The build failed with "No such file or directory: TINYCRYPT_DIR: '/home/zephyr/mcuboot_review/mcuboot/samples/ext/tinycrypt/lib'

Clearly, $mcuboot/boot/zephyr/CMakeLists.txt is not designed to be included`

So use ExternalProject_Add() or add_subdirectory().

Notice that both require a path. This is a problem for my use case, where the top-level CMakeLists.txt and configurations exist in a different repository than mcuboot or zephyr, and no command line options other than project path is allowed, and I am not allowed to change the mcuboot repository. (In my case even the board needs to be set through a file under SCM control.)

AFAICT, your suggestions still do not support my use case (building from a config entirely in a different repo).

You can create your own custom CMakeLists.txt and fully control MCUboot any way you like

I can, but I shouldn't have to. I am following a widely-presumed use case.

But #1489 is a PR where you propose yet one more variant in customization.

Unlike APPLICATION_CONFIG_DIR, the pattern of customization in this PR is not new to the Zephyr Project. Rather, this PR refactors the implementation to properly support Zephyr's standard pattern of customization. It also aligns with Zephyr's definition of "application", which does not require any application-unique logic.

How about this: Please show me how build and configure a bootloader build from a CMakeLists.txt and prj.conf under example-application (e.g., west build -b nrf52840dk_nrf52840 $example-application/mcuboot_external_configuration), because I don't think you can do it without modifying zephyr/ or mcuboot/, or putting restrictions on the workspace directory tree.

How about this: Please show me how build and configure a bootloader build from a CMakeLists.txt and prj.conf under example-application (e.g., west build -b nrf52840dk_nrf52840 $example-application/mcuboot_external_configuration), because

This is a PR review, so basically such a request should be posted at the proper channel, but as it just requires 5 CMake commands cmake_minimum_required(), project(), find_package(), include(), and ExternalProject_Add() then I have done you the courtesy of creating a repo with a freestanding application.
If it can work as a freestanding application, then it can also work as a workspace application.
You need to add this commit on top of mcuboot #1522 .
If this fails for you, please read the getting started on how to properly install Zephyr, or take any further questions to the proper channel (Discord).

See here:
https://github.com/tejlmand/oot_mcuboot_showcasing

$ cmake -GNinja -DBOARD=nrf52840dk_nrf52840 -Bbuild .
Loading Zephyr module(s) (Freestanding): west zephyr_module
...
-- Build files have been written to: /tmp/oot_mcuboot_external_configuration/build
$ ninja -C build
ninja: Entering directory `build'
[5/8] Performing configure step for 'mcuboot'
Loading Zephyr default modules (Zephyr workspace).
-- Application: /projects/github/ncs/bootloader/mcuboot/boot/zephyr
...
-- Found devicetree overlay: /tmp/oot_mcuboot_external_configuration/app.overlay
...
Loaded configuration '/projects/github/ncs/zephyr/boards/arm/nrf52840dk_nrf52840/nrf52840dk_nrf52840_defconfig'
Merged configuration '/tmp/oot_mcuboot_external_configuration/prj.conf'
Merged configuration '/tmp/oot_mcuboot_external_configuration/boards/nrf52840dk_nrf52840.conf'
...
271/271] Linking C executable zephyr/zephyr.elf
Memory region         Used Size  Region Size  %age Used
           FLASH:       40092 B        48 KB     81.57%
             RAM:       23808 B       256 KB      9.08%
        IDT_LIST:          0 GB         2 KB      0.00%
[8/8] Completed 'mcuboot'

I don't think you can do it without modifying zephyr/ or mcuboot/, or putting restrictions on the workspace directory tree.

As I have said before, I don't think you have fully understood the power of the current design and infrastructure of the build system.

Can we close this topic and PR now ?

Thank you for the example. I did not want to assume that ${ZEPHYR_<moduleNameUpper}_MODULE_DIR} was always going to be available for use in ExternalProject_Add()`.

I was able to successfully build this stand-alone, when added into example-application as the manifest repositoryl

BUT ...

The build failed when I also passed in -DCONF_FILE=.... Here are the results I got:

• "-DCONF_FILE=prj.conf": picks up local prj.conf; does NOT pick up either local or remote boards/nrf52840dk_nrf52840.conf; did not generate the proper size image.
• "-DCONF_FILE=${ZEPHYR_MCUBOOT_MODULE_DIR}/boot/zephyr/prj.conf": picks up $mcuboot/boot/zephyr/prj.conf; does NOT pick up either local or remote boards/nrf52840dk_nrf52840.conf; did not generate the proper size image
• "-DCONF_FILE=${ZEPHYR_MCUBOOT_MODULE_DIR/boot/zephyr/prj.conf;prj.conf": picks up $mcuboot/boot/zephyr/prj.conf; does NOT pick up local prj.conf does not pick up remote and local boards/nrf52840dk_nrf52840.conf; did not generate the proper size image.

So ... both approaches suffer from common failures in the build system forcing the duplication of config files/settings.

Now we're down to comparing the actual approaches to support reuse of logic currently private to an application. We're making headway!

First, notice that Zephyr's glossary definition of application does not require any logic or data unique to it. As you know, the build system works fine if an application provides a zero-byte C source file. We need to discuss the reuse of logic (e.g., library/subsystem) separate from the reuse of an application (configuration and build files).

Second, in general no one is able to predict when logic in one application will need to be reused in another. Having experienced the evolution and growth of a software product line over many generations. I am a witness that functionality we never expected to be used in a different application needed to be refactored out for sharing. The strategic direction is to expect any/all logic will end up being shared across multiple applications.

Third, the mcuboot module already exposes bootutil as a subsystem through module.yml. It is already configurable through Kconfigs. The bootutil subsystem/library already has a zephyr unique CMakeLists.txt. None of these concepts are introduced with this PR. In fact, this PR is refactoring the boot/zephyr/ build files to follow these existing concepts and patterns to expose the logic to be reused in the existing/expected way.

Fourth, it sounds like APPLICATION_CONFIG_DIR was introduced as a hotfix for a specific use case. The pattern implied by this functionality is to attempt to reuse logic and build files that may not have been designed for reuse. (See above issues.) I expect this tactic was straight forward but strategically leads to a more fragile ecosystem than one where all code to reuse is created and maintained as intentionally reusable code.

With all that I have learned about your recommendations and experienced myself over the years, it seems to me having MCUboot refactor out reusable code into subsystems (as done in this PR) is the wiser solution for both the short and long term.

Evaluating the complexity of this PR in terms of touched file count is really inappropriate. Whether one file is moved or an entire directory is should not matter. This PR is simply:

1. splitting files apart based on fragments of reuse
2. locating the reusable portions in the expected location for reusable content
3. providing a standard sample/test case for automatic (re)verification
4. putting all this in the organizational structure that is naturally understood/expected/debuggable by all working in the Zephyr ecosystem.

Honestly, your steadfast advocacy of APPLICATION_CONFIG_DIR sounds like I don't understand / like the current zephyr platform architecture therefore I'll promote this structure instead. This will likely cause you problems when the build system has to be "qualified" for the auditable branch. You, my friend, will have to tackle the whole of software engineering to do that. You might as well take the free course LFD-116 sooner rather than later.

Now, are you ready to approve this PR so we can move on to filling out mcuboot test cases?

"-DCONF_FILE=prj.conf": picks up local prj.conf; does NOT pick up either local or remote boards/nrf52840dk_nrf52840.conf; did not generate the proper size image.

Did you fail to read this:

If this fails for you, please read the getting started on how to properly install Zephyr, or take any further questions to the proper channel (Discord).

that's how -DCONF_FILE is documented to work.
Using CONF_FILE is opting-out of the automatic process.
go read the docs:
https://docs.zephyrproject.org/latest/build/kconfig/setting.html#the-initial-configuration
or read the comment I gave you here:
#1524 (comment)

it sounds like APPLICATION_CONFIG_DIR was introduced as a hotfix for a specific use case.

Quite the opposite, it's generic and scalable as it works for any app, from hello world, to hci_rpmsg, and even to MCUboot.
And can be used for any board.

Your proposal is MCUboot specific and tailored exactly to your needs.
It makes application specific Kconfigs available in the general Kconfig tree, not only MCUboot app but all applications now has those settings available.

Imagine each and every sample in Zephyr want to have their settings available in the global Kconfig space.
What a mess and nightmare that will become.

that's how -DCONF_FILE is documented to work.

From the perspective of multiple stakeholders in Zephyr, your design is not valid no matter how much it has been verified. See below.

(The design also doesn't fully meet your specification, as it didn't properly parse a list of multiple prj.conf.)

Quite the opposite, it's generic and scalable as it works for any app, from hello world, to hci_rpmsg, and even to MCUboot.
And can be used for any board.

The design of CONF_FILE rules do not support the SOLID design principles, specifically the Liskov Substitution Principle. Thus it is not scalable to multiple levels of project derivation. In case you hadn't noticed it the Zephyr ecosystem architecture is a client-server organized architecture of objects. Notice how:

• In https://docs.zephyrproject.org/latest/develop/api/design_guidelines.html
  • the first bullet is related to providing an instance reference to the object related to the callback,
  • the third bullet a reference to is originator data.
• the SYS_INIT() interface hooks in the object constructor

Your proposal is MCUboot specific and tailored exactly to your needs.

How does aligning with the general pattern of Zephyr modularity make it specific and tailored to my needs? Try again.

It makes application specific Kconfigs available in the general Kconfig tree, not only MCUboot app but all applications now has those settings available.

Your approach does the same thing in a less obvious way. The oot configuration and CMakeLists.txt constitutes a separate application per Zephyr's definition. ANY/all applications already have them available. Once the code is reused the settings are no longer application specific. My approach exposes the Kconfigs controlling the reusable logic in exactly the same way as happens in other downstream modules - following the solution supported by Zephyr.

Anyone need to be able to combine the MCUboot app logic with a ztest suite. By the Zephyr definition this is a different application. I also need to be able to combine MCUboot app logic with other proprietary logic - without touching the MCUboot repository. Fundamentally, all certifiable logic will be shared with test applications. The typical case will be sharing that logic with a ztest application.

Imagine each and every sample in Zephyr want to have their settings available in the global Kconfig space.
What a mess and nightmare that will become.

It doesn't have to be, and Zephyr header include guards have already illustrated the solution. The Kconfigs simply need to be prefixed with the moduleName+pathToDefinition. This pattern of solution has been practiced for decades.

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.